Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

External Library Models: Adding support for @Nullable Method parameters #1006

Merged
merged 47 commits into from
Sep 23, 2024

Conversation

akulk022
Copy link
Collaborator

@akulk022 akulk022 commented Jul 22, 2024

With these changes we can read annotations from method parameters and use them with our External Library Models in the following manner.

Externally annotated code example:

@NullMarked
public class ParameterAnnotationExample {

    public static Integer add(Integer a, Integer b) {
        return a + b;
    }

    public static Object getNewObjectIfNull(@Nullable Object object) {
        if (object == null) {
            return new Object();
        } else {
            return object;
        }
    }
}
class Test {
    static ParameterAnnotationExample annotationExample = new ParameterAnnotationExample();
    static void testPositive() {
      // BUG: Diagnostic contains: passing @Nullable parameter 'null' where @NonNull is required
      annotationExample.add(5,null);
    }
    static void testNegative() {
      annotationExample.getNewObjectIfNull(null);
    }
}

In order to accomplish this, we had to change the astubx format to use fully-qualified names for argument and return types. This also required changes to JarInfer. These are backward-incompatible changes, so we have changed the magic number for astubx files.

@akulk022 akulk022 marked this pull request as draft July 22, 2024 17:45
@msridhar
Copy link
Collaborator

I wanted to summarize some thoughts and issues on how to move forward with this PR, to possibly get some help.

As part of #950, we need a way to store in an astubx file the explicitly @Nullable method parameters in a @NullMarked class; then, all other method parameters in that class can be properly treated as @NonNull. (We already added support for storing which classes are @NullUnmarked in a stub file.) The code to make use of this stub information is around here in LibraryModelsHandler, and this code requires that the library models have fully-qualified parameter type names for its lookups. Before, for some reason, we only stored simple type names for parameters in stub files. So, we went ahead and modified our stub file writing logic to try to write fully-qualified type names.

But this change, in turn, caused problems for JarInfer, as its logic for storing stub files still uses simple type names, but we are using shared parsing logic for stub files. (At least I think that's the issue; @akulk022 can you clarify why you had to make the change in InferredJARModelsHandler at line 246?) Bottom line this is a backwards-incompatible change to the astubx format, so we need to be careful with it.

I propose the following:

  • We use fully-qualified type names wherever possible. Not using fully-qualified names is potentially broken in the presence of method overloading; we might as well get it right.
  • We bump the file version magic number for astubx files that are written and include fully-qualified names.

A question becomes, what should we do with old astubx files that come in? In particular, I'm concerned about the astubx files in the android-jarinfer-models-sdk* artifacts. I think we either need to somehow support old astubx file versions through some "slow path" where we do a lookup based on simple type names, or we need to update the astubx files in those artifacts before the next release. I'm hoping we don't need to do both things, as I'm guessing if we update those artifacts, that will handle nearly all use of astubx in the wild, and for any other users they can just re-generate their astubx files using the latest release to update.

@lazaroclapp do you have any thoughts / feedback on this?

@akulk022
Copy link
Collaborator Author

@msridhar I made the change on line 246 in InferredJARModelsHandler.java because the existing test case libraryModelNullableReturnsTest was failing since argAnnotCache now contains fully qualified names for the parameters and we were not able to perform a lookup in the cache after getting a simple type name. Do you think we should separate out the logic for handling @Nullable returns for Inferred JAR Models and Library Models?

@msridhar
Copy link
Collaborator

msridhar commented Jul 25, 2024 via email

@lazaroclapp
Copy link
Collaborator

I propose the following:

  • We use fully-qualified type names wherever possible. Not using fully-qualified names is potentially broken in the presence of method overloading; we might as well get it right.
  • We bump the file version magic number for astubx files that are written and include fully-qualified names.

A question becomes, what should we do with old astubx files that come in? In particular, I'm concerned about the astubx files in the android-jarinfer-models-sdk* artifacts. I think we either need to somehow support old astubx file versions through some "slow path" where we do a lookup based on simple type names, or we need to update the astubx files in those artifacts before the next release. I'm hoping we don't need to do both things, as I'm guessing if we update those artifacts, that will handle nearly all use of astubx in the wild, and for any other users they can just re-generate their astubx files using the latest release to update.

@lazaroclapp do you have any thoughts / feedback on this?

I agree with the approach above, and we can even just error out if the astubx file doesn't match the magic number we expect (i.e. the next NullAway version only supports the android-jarinfer-models-sdk* artifacts that have the same version number, unless supporting both versions is trivial). Normally I'd be very hesitant to suggest this, but:

  • In current versions of JarInfer, the preferred mode has long been to rewrite the bytecode of the jar files, rather than generate .astubx files, except for the Android SDK models.
  • Since that's the only astubx jar we expect to see in practice (other than the JSpecify models), and we version it together with the main NullAway jar, we might as well require people to update them concurrently, so long as we don't silently fail.

This will imply regenerating android-jarinfer-models-sdk* for all the Android SDK versions we have historically supported (and ideally we should update the models to more recent AOSP/SDK versions, btw, but I know that's a bit of a pain).

Does this seem reasonable?

@msridhar
Copy link
Collaborator

Thanks @lazaroclapp! All sounds good to me. My only concern is the following:

This will imply regenerating android-jarinfer-models-sdk* for all the Android SDK versions we have historically supported (and ideally we should update the models to more recent AOSP/SDK versions, btw, but I know that's a bit of a pain).

Thinking more about this, will this involve building each of those SDK versions from source? That may be moderately to extremely painful...we can look into it. But I agree it's the best solution and we should see if we can do it.

@msridhar
Copy link
Collaborator

I'd like to unblock this PR. I propose we go ahead and evolve the astubx format with fully-qualified types, and I will find a way to update the android-jarinfer-models-sdk* as needed. I'm wondering if the following two-step process is best:

  1. In a separate PR, get rid of InferredJarModelsHandler and consolidate any extra logic into the LibraryModelsHandler code.
  2. Rebase this PR on top of that one, and here we do the evolution of the astubx format and update the magic number.

@akulk022 WDYT? If you think it's ok you can go ahead with 1 as a separate PR (or I can potentially look into it).

@msridhar
Copy link
Collaborator

msridhar commented Sep 4, 2024

Update here is that it's hard to get rid of InferredJarModelsHandler without first switching to fully-qualified types in astubx files. The logic here for looking up method parameter nullability strips down parameter types to simple names, which isn't compatible with how the standard handling of library models first constructs an optimized representation (based on fully-qualified types) and then does lookups into it. So at this point, I think it makes more sense to push forward this PR, which switches over to fully-qualified types, and then get rid of InferredJarModelsHandler.

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 93.82716% with 5 lines in your changes missing coverage. Please review.

Project coverage is 87.54%. Comparing base (3418a6c) to head (e1cbcf9).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
.../uber/nullaway/libmodel/LibraryModelGenerator.java 91.83% 1 Missing and 3 partials ⚠️
...ava/com/uber/nullaway/handlers/StubxCacheUtil.java 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1006      +/-   ##
============================================
+ Coverage     87.23%   87.54%   +0.31%     
- Complexity     2129     2138       +9     
============================================
  Files            83       83              
  Lines          6965     7003      +38     
  Branches       1356     1367      +11     
============================================
+ Hits           6076     6131      +55     
+ Misses          458      451       -7     
+ Partials        431      421      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

msridhar added a commit that referenced this pull request Sep 6, 2024
This is in preparation for #1006. When working on that PR I realized we
really need some unit tests to more easily debug various scenarios
(right now all we have are integration tests). Plus this will make our
code coverage reports more useful. This adds some infrastructure for
unit tests and a couple of basic tests; we will add more as we go. Also
fix a couple minor issues in `LibraryModelGenerator`.
Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking promising! I have a few comments

private final Map<String, Set<Integer>> nullableUpperBounds;
// TODO this is sketchy!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this comment 🙂 I think we need to do for arrays whatever JarInfer is now doing. We should also add integration tests.

ResolvedType returnType = md.getReturnType();
if (returnType.isReferenceType()) {
return returnType.asReferenceType().getQualifiedName().toString();
} else if (returnType.isArray()) {
return ARRAY_RETURN_TYPE_STRING;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this needs to be updated

Comment on lines 109 to 114
"@NullMarked",
"public class NullMarked{",
"//Only this class should appear under null marked classes",
"}",
"class NotNullMarked{",
"}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation

};
ImmutableSet<String> expectedNullMarkedClasses = ImmutableSet.of("NullMarked");
runTest(
"NullableUpperBound.java",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird file name. Choose one of the classes actually included in the file.

@@ -1287,20 +1287,23 @@ private <T> NameIndexedMap<T> makeOptimizedLookup(
/** Constructs Library Models from stubx files */
private static class ExternalStubxLibraryModels implements LibraryModels {

@SuppressWarnings("unused")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get rid of suppression or delete the field

Comment on lines 1336 to 1338
if (index >= 0) {
mapBuilder.put(methodRef(className, methodName), index);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to check if index >= 0? And don't we need to check if the annotation is @Nullable? This seems to add something to the map for any annotation. If we can, we should add a test so that if the annotation is @NonNull we skip it (not sure if this is possible).

Copy link
Collaborator Author

@akulk022 akulk022 Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the index>=0 check because argAnnotCache holds the annotation for the return of the method with the index -1 and for all the parameter annotations with their respective indices, should this have been handled in a different way? I can add the @Nullable check and the corresponding test case.

Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting pretty close!

* The file magic number for version 1 .astubx files. It should be the first four bytes of any
* compatible .astubx file.
*/
private static final int VERSION_1_FILE_MAGIC_NUMBER = 481874642;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lazaroclapp should we have a separate magic number (for the astubx file format) and version number?

Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution @akulk022!

@msridhar msridhar merged commit f36ab25 into uber:master Sep 23, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants